-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disk buffers #109
Disk buffers #109
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #109 +/- ##
==========================================
- Coverage 72.30% 72.00% -0.30%
==========================================
Files 75 78 +3
Lines 4538 5050 +512
==========================================
+ Hits 3281 3636 +355
- Misses 973 1056 +83
- Partials 284 358 +74
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only reviewed the interface & docs so far, but will circle back to the rest later today. Here's feedback on what I've reviewed so far though.
|
||
| Field | Default | Description | | ||
| --- | --- | --- | | ||
| `max_size` | `4294967296` (4GiB) | The maximum size of the disk buffer file in bytes | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this is documented well here, it might be slightly clearer to call this max_bytes
, since many users will only encounter this in a config file and would either have to make assumptions about the units, or dig up the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially thought the same thing, but I would love to implement something like this as a future feature. Would it still make sense to do max_bytes
if the value is 8GB
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I guess it comes down to how likely we are to implement that feature and how soon it would happen. If we're going to end up living with the current implementation for a while, then I think we should consider using the more explicit term now and deprecating it later. We could pretty easily support both and just require at most one of the two be specified.
|
defer d.Unlock() | ||
defer func() { d.lastCompaction = time.Now() }() | ||
|
||
// So how does this work? The goal here is to remove all flushed entries from disk, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😮 Nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me. Nice job on the detailed design. Awesome test coverage.
Description of Changes
Adds a disk buffer, a new memory buffer, and a flusher.
Things that could use specific review effort (obviously in addition to anything you'd like to add):
Trying to at least somewhat limit the scope, there are still a couple of features left undone:
Please check that the PR fulfills these requirements